Skip to content

PMM-15216 Migrate the SEP UI into PMM - #5653

Draft
fabio-silva wants to merge 3 commits into
PMM-15288from
PMM-15216
Draft

PMM-15216 Migrate the SEP UI into PMM#5653
fabio-silva wants to merge 3 commits into
PMM-15288from
PMM-15216

Conversation

@fabio-silva

@fabio-silva fabio-silva commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Ticket number: PMM-15216

Feature build: SUBMODULES-0

Stacked PR. Base is #5728 (PMM-15288, the pnpm + oxlint/oxfmt + library upgrade). Review that one first; the diff shown here is only the UI migration on top of it. This branch was rebuilt from main to split the two, so the commit history changed — the previous tip was 9a507d0c.

What

Brings SEP's frontend into ui/ and mounts the migrated plugins as native PMM routes, so SEP surfaces render inside the PMM shell instead of an iframe.

Packages ported from SEP's frontend workspace

PMM path From
ui/packages/sep/api typed API client, generated OpenAPI surfaces, hooks
ui/packages/sep/framework schema-driven form/list/task components
ui/packages/sep/shared shared primitives
ui/packages/plugins/atw Collect Diagnostic Data (ATW)

SEP's "app" vocabulary is renamed to "plugin" throughout the port, since "app" already means a workspace app in ui/: SchemaDrivenAppSchemaDrivenPlugin, useAppSchemausePluginSchema, useAppTasksusePluginTasks, app-schema.tsplugin-schema.ts. Ported files also carry PMM's AGPL header.

Wiring in apps/pmm

  • router.tsx mounts the plugins under their own routes; SepPage gives them the standard PMM Page chrome (padding, width, auth gate, footer).
  • Navigation gates the SEP entries behind admin + the inventory settings flag.
  • main.tsx calls initSepAuth, pointing SEP's axios client at PMM's session. Auth is still the interim Option D: the dev proxy injects SEP_INTERNAL_TOKEN server-side, so no token reaches the browser.
  • vite.config.ts proxies SEP's paths (/api, /sep_app, /stream-logs, /execution-events, /files) to SEP_BACKEND_URL, and lets PMM_SERVER_URL override the PMM target.
  • A SyntaxHighlighter component backs the schema renderer's script/JSON fields.
  • Page gains maxWidth so SEP pages can opt into the full-width container from @percona/percona-ui; Settings.tsx moves to it in place of the removed fullWidth flag.

SEP catch-up (second commit)

SEP moved on after the initial port. Ported commit by commit rather than by copying files, so the app→plugin rename and license headers survive:

  • SEP-1629 / SEP-1684 / SEP-1689 / SEP-1696 / SEP-1668 — refresh the generated OpenAPI surface from SEP head. Two schema components are now namespaced: ConnectivityWarningframework__ConnectivityWarning, TaskExecuteWriteframework__TaskExecuteWrite; their consumers move with them.
  • SEP-1663 — honor HostRef / HostField.allow_custom. HostField passes it to HostSelector, which renders FreeSoloSelect instead of the closed AutoCompleteInput and commits a scalar id/string (including from cascade auto-select). FreeSoloSelect now resolves a stored string against option ids, not just labels, so string host ids like "nomad-1" display as their option.
  • SEP-1653 — hide the task-history Download files button unless the files API returns a non-empty listing. has_logs was the wrong signal: logs exist even when the output dir holds only the hidden .sep-run-result.json marker, leaving a dead download action. Probes are cached for 30s so the history table's poll loop doesn't re-hit the files API every tick.
  • SEP-1692 — add postSession / postSessionExchange to @sep/api. The exchange endpoint trades PMM's session cookie for a short-lived SEP bearer, which is what replaces the interim SEP_INTERNAL_TOKEN wiring (that token's service principal hardcodes is_admin = False and so 403s every admin-gated surface).

Deliberately not ported

  • SEP-1663's multi-host half (FreeSoloMultiSelect, MultiHostField) — PMM's snapshot has no multi-host selector to extend. Porting it means first porting SEP's multi-host feature, which is its own change.
  • Flipping bootstrap.ts to the token exchange (Option B) — only the @sep/api client surface lands here. Switching over needs a SEP backend carrying POST /api/oauth/session/exchange and live verification, so it gets its own ticket.

Review comments addressed

Also gitignores *.tsbuildinfo; one had been committed by accident.

Verification

From ui/, on this branch (i.e. including the base PR):

Check Result
pnpm install clean
make format-check (oxfmt) 769 files, all formatted
make lint (oxlint) 0 errors across all 6 packages
make test (vitest) 116 files passed, 1084 tests passed, 1/13 skipped
make build pmm + pmm-compat build
turbo run check-types clean

Not yet verified against a live PMM Server with a SEP backend attached — that needs a feature build, which is tracked below.

Related

If this PR adds, removes or alters one or more API endpoints, please review and update the relevant API documentation as well:

  • API Docs updated

Summary by CodeRabbit

  • New Features

    • Added Advanced Troubleshooting Workspace for incident creation, snippet collection, execution results, diagnostics sending, retries, and support-case tracking.
    • Added MySQL Backups and diagnostic-data navigation and routing.
    • Added reusable schema-driven forms, selectors, scheduling panels, task history tables, log viewers, syntax highlighting, and file downloads.
    • Added centralized authentication and API connectivity with improved error handling.
    • Updated page layouts with configurable content widths.
  • Documentation

    • Added API package usage, setup, code-generation, testing, and authentication guidance.
  • Tests

    • Added extensive coverage for incident workflows, forms, selectors, API behavior, scheduling, logs, and task execution.

@fabio-silva
fabio-silva requested review from a team as code owners July 16, 2026 10:14
@fabio-silva
fabio-silva requested review from 4nte, matejkubinec, mattiasimonato and maxkondr and removed request for a team July 16, 2026 10:14
@it-percona-cla

it-percona-cla commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Ignacio Durand seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 45.41%. Comparing base (7e531d4) to head (c49953f).

Additional details and impacted files
@@              Coverage Diff              @@
##           PMM-15288    #5653      +/-   ##
=============================================
+ Coverage      45.38%   45.41%   +0.02%     
=============================================
  Files            418      418              
  Lines          43334    43334              
=============================================
+ Hits           19669    19678       +9     
+ Misses         21725    21715      -10     
- Partials        1940     1941       +1     
Flag Coverage Δ
admin 34.96% <ø> (ø)
agent 51.02% <ø> (+0.10%) ⬆️
managed 44.99% <ø> (-0.01%) ⬇️
vmproxy 72.22% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread build/docker/rpmbuild/Dockerfile.el8 Outdated
dnf clean all && rm -rf /var/cache/dnf

# UI build uses pnpm (packageManager in ui/package.json); nodesource nodejs ships npm/yarn only.
RUN npm install -g pnpm@11.1.3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to pass ppm version during docker build (read from some file in repo)? It will simplify the maintenance in future (less files are required to be changed during version bump).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call — done, and the hardcoded version is gone entirely rather than being passed in as a build arg.

All three rpmbuild Dockerfiles now just do:

RUN corepack enable

corepack ships with nodesource's nodejs 22 and resolves pnpm from the packageManager field in ui/package.json at build time, so a pnpm bump only touches that one field. ui/Makefile's setup target does corepack enable pnpm for the same reason, and .devcontainer/setup.sh no longer pins a version either.

This moved to the split-out toolchain PR: #5728.

Comment thread build/docker/rpmbuild/Dockerfile.el9 Outdated
dnf clean all && rm -rf /var/cache/dnf

# UI build uses pnpm (packageManager in ui/package.json); nodesource nodejs ships npm/yarn only.
RUN npm install -g pnpm@11.1.3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same as above

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fix as on el8 — see the reply above. Applied to el8, el9 and hetzner-el9 in #5728.

@yyyyyyyan

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts in this pull request

Copilot AI commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved by merging origin/main into this branch and fixing the conflicts in commit 9bf5e656e.

Copilot AI requested a review from yyyyyyyan July 30, 2026 01:29
Comment thread build/docker/rpmbuild/Dockerfile.el8 Outdated
dnf clean all && rm -rf /var/cache/dnf

# UI build uses pnpm (packageManager in ui/package.json); nodesource nodejs ships npm/yarn only.
RUN npm install -g pnpm@11.1.3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need both pnpm and yarn?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes — they cover different projects, and both are built from this image:

  • ui/ (the PMM UI workspace) is pnpm.
  • dashboards/pmm-app (the QAN Grafana plugin) is still yarn 1 — packageManager: yarn@1.22.21, and dashboards/Makefile runs yarn install --frozen-lockfile.

Migrating pmm-app off yarn is out of scope here, so the image needs both until that happens.

I've made the Dockerfile say so, and dropped the hardcoded pnpm version at the same time (per @maxkondr's comment above):

# The UI workspace (ui/) builds with pnpm; the QAN Grafana plugin (dashboards/pmm-app)
# is still on yarn 1, so both are needed. corepack ships with nodesource's nodejs 22
# and resolves pnpm from the `packageManager` field in ui/package.json, so the version
# is pinned in exactly one place and needs no change here on a bump.
RUN corepack enable

This moved to the split-out toolchain PR: #5728.

Comment thread build/docker/server/entrypoint.sh Outdated
is_enabled() { [ "$1" = "1" ] || [ "$1" = "true" ]; }
declare POSTGRES_DATA_DIR="/srv/postgres14"
declare POSTGRES_PASSWORD_FILE="/srv/.postgres_password"
declare POSTGRES_BIN_DIR="/usr/pgsql-14/bin"

@ademidoff ademidoff Aug 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabio-silva I don't really understand what this PR has to do with the entrypoint and postgres-migration.

Please remove that code, it's a separate PR that needs to be tested stand-alone and merged separately, possibly even prior to this one.

#5700

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, and removed. Neither branch touches that code any more — build/docker/server/entrypoint.sh, build/ansible/roles/postgres/* (including the new postgres-sep script), docker-compose.yml, .env.example and managed/utils/envvars are all gone from the diff. It's yours to land stand-alone in #5700.

While removing it we also split the rest of the PR in two, since the toolchain change and the UI migration were reviewing as one 320-file diff:

That meant rebuilding this branch from main, so its history changed; the previous tip was 9a507d0c.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@Nailya Let's give priority to testing #5700, we need it merged asap.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 746660af-01e2-469b-bb1c-ad52c1e0ef25

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)

Comment @coderabbitai help to get the list of available commands.

@nachodd nachodd changed the title PMM-15216 fix ui build: bootstrap pnpm via corepack, drop yarn leftover PMM-15216 Migrate the SEP UI into PMM Aug 3, 2026
@nachodd
nachodd changed the base branch from main to PMM-15288 August 3, 2026 17:20
Ignacio Durand added 2 commits August 3, 2026 16:37
Brings SEP's frontend packages into ui/ and mounts the migrated plugins as
native PMM routes, so SEP surfaces render inside the PMM shell instead of an
iframe. Builds on PMM-15288, which moved the workspace to pnpm and the
library versions SEP's code targets.

Packages, ported from SEP's frontend workspace:
- packages/sep/api       — typed API client, generated OpenAPI surfaces, hooks
- packages/sep/framework  — schema-driven form/list/task components
- packages/sep/shared     — shared primitives
- packages/plugins/atw    — Collect Diagnostic Data (ATW)

SEP's "app" vocabulary is renamed to "plugin" throughout the port, since "app"
already means a workspace app in ui/: SchemaDrivenApp -> SchemaDrivenPlugin,
useAppSchema -> usePluginSchema, useAppTasks -> usePluginTasks, app-schema.ts
-> plugin-schema.ts. Ported files also carry PMM's AGPL header.

Wiring in apps/pmm:
- router.tsx mounts the plugins under their own routes; SepPage gives them the
  standard PMM Page chrome (padding, width, auth gate, footer).
- navigation gates the SEP entries behind admin + the inventory settings flag.
- main.tsx calls initSepAuth, which points SEP's axios client at PMM's session.
  Auth is still the interim Option D: the dev proxy injects SEP_INTERNAL_TOKEN
  server-side, so no token reaches the browser.
- vite.config.ts proxies SEP's paths (/api, /sep_app, /stream-logs,
  /execution-events, /files) to SEP_BACKEND_URL, and lets PMM_SERVER_URL
  override the PMM target.
- A SyntaxHighlighter component backs the schema renderer's script/JSON fields.

Page gains maxWidth so SEP pages can opt into the full-width container from
@percona/percona-ui; Settings.tsx moves to it in place of the removed
fullWidth flag.

*.tsbuildinfo is gitignored; one had been committed by accident.

Signed-off-by: Ignacio Durand <ignacio.durand@l7informatics.com>
Catches the migrated packages up with SEP's frontend, which moved on after
the initial port. Ported commit by commit rather than by copying files, so
PMM's app -> plugin rename and license headers survive.

- SEP-1629 / SEP-1684 / SEP-1689 / SEP-1696 / SEP-1668: refresh the generated
  OpenAPI surface (specs/*.json + src/generated/*.ts) from SEP head. Two
  schema components are now namespaced — ConnectivityWarning and
  TaskExecuteWrite became framework__ConnectivityWarning and
  framework__TaskExecuteWrite — so their consumers move with them.
- SEP-1663: honor HostRef/HostField `allow_custom`. HostField passes it to
  HostSelector, which renders FreeSoloSelect instead of the closed
  AutoCompleteInput and commits a scalar id/string (including from cascade
  auto-select). FreeSoloSelect resolves a stored string against option ids,
  not just labels, so string host ids like "nomad-1" display as their option.
  SEP's multi-host half (FreeSoloMultiSelect, MultiHostField) is not ported —
  PMM's snapshot has no multi-host selector to extend.
- SEP-1653: hide the task-history Download files button unless the files API
  returns a non-empty listing. `has_logs` was the wrong signal: logs exist
  even when the output dir holds only the hidden .sep-run-result.json marker,
  which left a dead download action. Probes are cached for 30s so the history
  table's poll loop does not re-hit the files API every tick.
- SEP-1692: add postSession / postSessionExchange to @sep/api. The exchange
  endpoint trades PMM's session cookie for a short-lived SEP bearer, which is
  what replaces the interim SEP_INTERNAL_TOKEN wiring — that token's service
  principal hardcodes is_admin = False and so 403s every admin-gated surface.
  Only the client surface lands here; flipping bootstrap.ts over to it needs
  a SEP backend carrying the endpoint and is left to its own change.

Signed-off-by: Ignacio Durand <ignacio.durand@l7informatics.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

🟠 Major comments (30)
ui/packages/sep/api/tests/typed-client.test.ts-18-23 (1)

18-23: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Co-locate this test with typed-client.ts.

Move this file to ui/packages/sep/api/src/typed-client.test.ts. Update ui/packages/sep/api/vitest.config.ts so Vitest discovers the co-located test files.

As per coding guidelines, “Co-locate test files next to the components they test, using *.test.tsx or *.test.ts naming.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/api/tests/typed-client.test.ts` around lines 18 - 23, Move
the typed-client test from the tests directory to be co-located with
typed-client.ts as typed-client.test.ts, preserving its existing coverage and
imports. Update vitest.config.ts so Vitest includes co-located *.test.ts and
*.test.tsx files under the source tree.

Source: Coding guidelines

ui/packages/sep/api/src/client.ts-136-159 (1)

136-159: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

A synchronous throw from _onRefreshed still fails the refresh.

The comment on Lines 137-140 states that a synchronous throw from the injected _onRefreshed handler must not be reported as a failed refresh. The call on Line 157 sits inside the async executor, so a throw rejects refreshInFlight. Every awaiting caller then receives a rejection instead of the new token, and the 401 interceptor propagates a non-ApiError without retrying and without calling _onUnauthorized. Isolate the handler call to match the documented intent.

🛡️ Proposed fix to isolate the handler
-      _onRefreshed(data.access_token, data.expires_in);
+      try {
+        _onRefreshed(data.access_token, data.expires_in);
+      } catch {
+        // A failing auth-layer handler must not invalidate a successful
+        // cookie rotation.
+      }
       return data.access_token;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/api/src/client.ts` around lines 136 - 159, Isolate the
synchronous _onRefreshed call from the refreshInFlight async executor so its
exception cannot reject the shared refresh promise. Update the refresh flow
around _onRefreshed to invoke the handler after the promise has resolved, while
preserving the successful token return and existing network-error handling.
ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsx-166-170 (1)

166-170: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Guard schema.list_view before dereferencing it. PluginSchema.list_view is optional. An unknown entity route sets multi to false, then PluginListPage reads listView.columns and can crash. The detail route can also reach OverviewTab, where schema.list_view!.columns can crash.

  • Render Not found when the entity is unresolved and no top-level list_view exists.
  • Omit the Task information card when schema.list_view is absent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginListPage.tsx`
around lines 166 - 170, Guard the unresolved-entity path in PluginListPage so it
renders Not found when multi is false and schema.list_view is absent before
accessing listView.columns; preserve normal rendering when a list view exists.
In PluginDetailPage’s OverviewTab, conditionally omit the Task information card
when schema.list_view is unavailable instead of dereferencing
schema.list_view!.columns.
ui/packages/sep/framework/src/components/ScheduledTasksPanel/ScheduledTasksPanel.tsx-89-106 (1)

89-106: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

A simple enable/disable toggle destroys kwargs on the server.

handleToggleEnabled sends kwargs: '{}' in a full PUT. Any scheduled task created with non-default kwargs loses that data when a user flips the switch. The inline comment records the backend gap, but the current code turns a read-only-looking UI action into silent data loss.

Consider one of these mitigations until the backend exposes kwargs in PeriodicTaskResponse:

  • Preserve the value when it is present on the response object, and only fall back to '{}' when it is absent.
  • Disable the toggle for tasks whose kwargs cannot be round-tripped.

I can open a tracking issue for the backend schema gap if that helps.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/ScheduledTasksPanel/ScheduledTasksPanel.tsx`
around lines 89 - 106, Update handleToggleEnabled to preserve task.kwargs when
the response object provides it, using '{}' only as the fallback when kwargs is
absent. Keep the existing PeriodicTaskUpdate payload and toggle behavior
unchanged otherwise, and retain the fallback until PeriodicTaskResponse exposes
kwargs consistently.
ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx-300-315 (1)

300-315: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Define shared SEP route constants.

The navigation builder and router define the same SEP route values independently. Define the route values in src/lib/constants.ts and consume them from both sites.

  • ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx#L300-L315: use shared constants for url and matches.
  • ui/apps/pmm/src/router.tsx#L88-L107: use the same constants for route paths and routeBase.

As per coding guidelines, “Do not hardcode URLs; use constants from src/lib/constants.ts.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx` around lines 300 -
315, Define shared SEP route constants in src/lib/constants.ts, then update
addSepApps in ui/apps/pmm/src/contexts/navigation/navigation.utils.tsx to use
them for url and matches, and update the SEP routes and routeBase in
ui/apps/pmm/src/router.tsx to use the same constants instead of hardcoded paths.

Source: Coding guidelines

ui/apps/pmm/src/sep/SepPage.tsx-14-19 (1)

14-19: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Enforce the PMM-admin gate at the route wrapper.

NavigationProvider hides SEP entries for non-admin users, but direct navigation still matches the SEP routes. SepPage does not pass a role restriction to Page, so every authorized user can render these plugins.

Apply the same PMM-admin authorization policy at SepPage or at each SEP route. Do not rely on navigation visibility as an authorization control.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/apps/pmm/src/sep/SepPage.tsx` around lines 14 - 19, Update the SepPage
component to pass the existing PMM-admin role restriction to the Page wrapper,
ensuring direct SEP route navigation is denied for non-admin users while
preserving the current children layout.
ui/apps/pmm/vite.config.ts-20-21 (1)

20-21: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use the development environment-variable prefix.

Rename SEP_BACKEND_URL to PMM_DEV_SEP_BACKEND_URL. Rename SEP_INTERNAL_TOKEN to PMM_DEV_SEP_INTERNAL_TOKEN. These variables configure only the Vite development proxy.

As per coding guidelines, “Use environment-variable prefixes consistently: PMM_DEV_* for development/testing only.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/apps/pmm/vite.config.ts` around lines 20 - 21, Update the
environment-variable references used by the Vite development proxy: rename
SEP_BACKEND_URL to PMM_DEV_SEP_BACKEND_URL and SEP_INTERNAL_TOKEN to
PMM_DEV_SEP_INTERNAL_TOKEN in the configuration initialization. Preserve the
existing proxy behavior and values.

Source: Coding guidelines

ui/apps/pmm/src/sep/bootstrap.ts-17-20 (1)

17-20: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Add production SEP routing and authentication.

The deployed Nginx configuration has no upstream or location for /api, /sep_app, /stream-logs, /execution-events, or /files. Only the Vite development proxy injects SEP_INTERNAL_TOKEN. Because initSepAuth returns null, deployed SEP requests have no SEP bearer and cannot work. Add production routing with server-side credentials, or wire postSessionExchange() before merge.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/apps/pmm/src/sep/bootstrap.ts` around lines 17 - 20, Update initSepAuth to
establish production SEP authentication instead of returning a null token, and
ensure deployed Nginx routing forwards /api, /sep_app, /stream-logs,
/execution-events, and /files with server-side SEP credentials. Reuse
postSessionExchange() if that is the intended authentication flow, and preserve
the existing unauthorized-handler registration.
ui/packages/plugins/atw/tests/CategoryBrowser.test.tsx-18-22 (1)

18-22: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Component tests are not co-located with their components. The ATW package keeps component tests in a separate tests/ directory and imports the component through ../src/.... The coding guidelines require co-location.

  • ui/packages/plugins/atw/tests/CategoryBrowser.test.tsx#L18-L22: move the file to ui/packages/plugins/atw/src/CategoryBrowser.test.tsx and change the import to ./CategoryBrowser.
  • ui/packages/plugins/atw/tests/ResultsPane.test.tsx#L18-L22: move the file to ui/packages/plugins/atw/src/ResultsPane.test.tsx and change the import to ./ResultsPane.

Keep ui/packages/plugins/atw/tests/setup.ts where it is, and confirm that ui/packages/plugins/atw/vitest.config.ts include globs still match the new locations.

As per coding guidelines: "Co-locate test files next to the components they test, using *.test.tsx or *.test.ts naming."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/plugins/atw/tests/CategoryBrowser.test.tsx` around lines 18 - 22,
Move CategoryBrowser.test.tsx to
ui/packages/plugins/atw/src/CategoryBrowser.test.tsx and update its
CategoryBrowser import to ./CategoryBrowser; likewise move ResultsPane.test.tsx
to ui/packages/plugins/atw/src/ResultsPane.test.tsx and update its import to
./ResultsPane. Keep ui/packages/plugins/atw/tests/setup.ts unchanged and verify
vitest.config.ts include globs cover the relocated tests.

Source: Coding guidelines

ui/packages/plugins/atw/src/CollectPane.tsx-62-81 (1)

62-81: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Flatten dotted ATW parameter names before building the payload.

ATW fields include source.path. The form stores this as nested objects, but buildBatchPayload passes those objects through toArgs unchanged. Flatten shared and per-snippet values back to their declared keys, and add regression coverage for both paths.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/plugins/atw/src/CollectPane.tsx` around lines 62 - 81, Update
buildBatchPayload and its toArgs flow to flatten nested form objects into the
declared dotted ATW parameter keys, including shared values and per-snippet
values such as source.path. Preserve existing argument conversion behavior for
non-dotted fields, and add regression coverage verifying both shared and
per-snippet payload paths produce flattened keys.
ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx-63-85 (1)

63-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

normalizeChoiceDefaults misses fields with dotted names.

flattenSectionFields also returns one_of branch fields, whose names are dotted paths such as source.mode (see the one_of cases in SchemaFormRenderer.test.tsx). This function reads and writes flat keys with out[field.name], so a nested choice value stored at { source: { mode: 'rsync' } } is never normalized. The case-mismatched value then reaches the form and renders as an empty selection, which is the failure this function exists to prevent.

Use the exported getAtPath and setAtPath helpers instead of flat key access. Note that setAtPath mutates nested objects, so deep-copy the input first.

🐛 Proposed fix using path-aware accessors
-import {
-  SchemaFormRenderer,
-  coerceFormValues,
-  flattenSectionFields,
-} from '../SchemaFormRenderer';
+import {
+  SchemaFormRenderer,
+  coerceFormValues,
+  flattenSectionFields,
+  getAtPath,
+  setAtPath,
+} from '../SchemaFormRenderer';
@@
-  const out = { ...form };
+  const out = structuredClone(form) as Record<string, unknown>;
   for (const field of flattenSectionFields(sections)) {
     if (field.type !== 'choice' && field.type !== 'multi_choice') {
       continue;
     }
     const choiceMap = new Map(
       field.choices.map((c) => [c.value.toLowerCase(), c.value])
     );
-    const raw = out[field.name];
+    const raw = getAtPath(out, field.name);
     if (field.type === 'multi_choice' && Array.isArray(raw)) {
-      out[field.name] = raw.map((v) => {
-        const canonical =
-          typeof v === 'string' ? choiceMap.get(v.toLowerCase()) : undefined;
-        return canonical ?? v;
-      });
+      setAtPath(
+        out,
+        field.name,
+        raw.map((v) => {
+          const canonical =
+            typeof v === 'string' ? choiceMap.get(v.toLowerCase()) : undefined;
+          return canonical ?? v;
+        })
+      );
     } else if (field.type === 'choice' && typeof raw === 'string') {
       const canonical = choiceMap.get(raw.toLowerCase());
       if (canonical !== undefined) {
-        out[field.name] = canonical;
+        setAtPath(out, field.name, canonical);
       }
     }
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx`
around lines 63 - 85, Update normalizeChoiceDefaults to deep-copy the input
form, then use the exported getAtPath and setAtPath helpers for reading and
writing each choice field in flattenSectionFields, including dotted one_of paths
such as source.mode. Preserve the existing choice and multi_choice
canonicalization behavior while ensuring setAtPath receives the normalized
value.
ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts-66-72 (1)

66-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

useCascadingField does not follow the form's empty-value contract. buildFormDefaults in SchemaFormRenderer.tsx (line 85) seeds service, schema, table, and host fields to ''. These are exactly the cascading selector types named in the doc comment on line 38. This hook instead treats undefined as the empty value, which breaks both the clear path and the readiness check. Pick '' as the single empty-value sentinel and apply it in both places.

  • ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts#L66-L72: change setValue(fieldName, undefined, …) to setValue(fieldName, '', …). undefined also flips a bound MUI input from controlled to uncontrolled, so React logs a warning and the field can keep showing the stale selection.
  • ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts#L75-L80: add upstreamValue !== '' to the ready expression. An upstream selector with nothing selected holds '' today, so ready returns true and a downstream selector fetches options for an empty parent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts`
around lines 66 - 72, The useCascadingField clear path and readiness check must
use '' as the form’s empty-value sentinel. In
ui/packages/sep/framework/src/components/SchemaFormRenderer/hooks/useCascadingField.ts
lines 66-72, update setValue in the previousRef change block to clear fieldName
with '' instead of undefined; in lines 75-80, update the ready expression to
require upstreamValue !== '' so downstream options are not fetched without a
selected parent.
ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.tsx-356-361 (1)

356-361: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

A section-violation block silently disarms the unsaved-changes guard.

handleFormSubmit returns without throwing when hasSectionViolations is true. react-hook-form treats that as a successful submit and sets formState.isSubmitSuccessful = true. useUnsavedChangesGuard computes isDirty && !isSubmitSuccessful (useUnsavedChangesGuard.ts line 33), so isGuarded becomes false. The re-arm effect in that hook only runs when submitError is truthy, and submitError stays null on this path. The guard therefore stays disarmed: the beforeunload prompt and UnsavedChangesBlocker no longer fire, and the user can navigate away and lose the form data.

Gate the submit before handleSubmit runs, or mark the form invalid so RHF does not flag the submit as successful.

🐛 Proposed fix — block in the submit event handler instead
   const handleSubmitEvent = (event: FormEvent<HTMLFormElement>) => {
+    if (hasSectionViolations) {
+      // Section-level rules already render their own inline Alerts. Stop here so
+      // react-hook-form never marks the submit successful, which would disarm
+      // useUnsavedChangesGuard.
+      event.preventDefault();
+      return;
+    }
     if (appliedServerErrorPaths.current.length > 0) {

Then drop the early return from handleFormSubmit.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/SchemaFormRenderer/SchemaFormRenderer.tsx`
around lines 356 - 361, Move the hasSectionViolations check out of
handleFormSubmit and gate the submit before react-hook-form handleSubmit runs,
so blocked submissions are not marked successful and the unsaved-changes guard
remains active. Remove the early return from handleFormSubmit while preserving
its existing onSubmit behavior for valid submissions.
ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx-386-398 (1)

386-398: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

SchemaListView.tsx bypasses PMM theme tokens in two places. Both sites inline a design value instead of reading it from the theme, so the schema-driven list does not follow pmmThemeOptions. The shared fix is to define the missing tokens in pmmThemeOptions from @percona/percona-ui and read them here.

  • ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx#L386-L398: replace bgcolor: 'common.white' on both muiTablePaperProps and muiTableContainerProps with bgcolor: 'background.paper', and fix the opacity of background.paper in pmmThemeOptions so every PMM surface benefits. Pinning common.white renders a white surface in dark mode, as the inline comment already acknowledges.
  • ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx#L146-L154: replace fontFamily: "'Roboto Mono', monospace" in the code branch with a monospace token read from the theme, or add a theme Typography variant for code cells and use that variant.

As per coding guidelines: "Do not use hard-coded colors, font families, or spacing that bypass the theme; prefer sx with theme tokens, breakpoints, and MUI Typography variants" and "Use ColorModeContext or existing hooks such as useColorMode for light/dark mode".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx`
around lines 386 - 398, Update SchemaListView.tsx at lines 386-398 so both
muiTablePaperProps and muiTableContainerProps use the theme token
background.paper, and update pmmThemeOptions to provide an opaque,
mode-appropriate background.paper value. At lines 146-154, replace the
hard-coded "'Roboto Mono', monospace" in the code branch with the theme’s
monospace token or a dedicated code Typography variant, and use that theme value
for code cells.

Source: Coding guidelines

ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx-108-116 (1)

108-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Guard undefined as well as null.

Line 112 checks only value === null. data is Record<string, unknown>[], and the columns come from a server-supplied schema, so a row can omit a declared column key. In that case value is undefined and line 115 produces the string 'undefined'.

The formats amplify this:

  • default and chip render the literal text undefined.
  • date renders Invalid Date.
  • relative renders NaNd ago, because every mins/hours comparison against NaN is false.

Use a loose null check so both cases fall to the em dash.

🐛 Proposed fix
 function formatCellValue(
   value: unknown,
   format: ListColumn['format']
 ): ReactNode {
-  if (value === null) {
+  if (value === null || value === undefined) {
     return '—';
   }
   const str = String(value);

Consider also guarding an unparsable date in the date and relative branches:

     case 'date':
-      return new Date(str).toLocaleDateString();
+    case 'date': {
+      const d = new Date(str);
+      return Number.isNaN(d.getTime()) ? '—' : d.toLocaleDateString();
+    }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/framework/src/components/SchemaListView/SchemaListView.tsx`
around lines 108 - 116, Update formatCellValue to use a loose null check so both
null and undefined values return the em dash before String conversion or format
handling. Preserve the existing behavior for defined values and formats.
ui/packages/sep/framework/src/hooks/useResolvedServiceField.ts-80-96 (1)

80-96: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Expose the fetch error state.

The hook discards the error state of useServices. If the services fetch fails, isFetched becomes true and services stays EMPTY_SERVICES. service is then undefined and isResolving is false.

Callers cannot distinguish two different situations:

  • The scalar id resolved to no matching service.
  • The fetch failed, so resolution never completed.

The doc comment at Lines 44-48 instructs callers to wait rather than treat the parent as missing. On a fetch failure the caller receives the exact signal it was told means "not resolving", so it renders a permanently blank service name with no error surfaced.

Add the error state to ResolvedServiceField.

🔧 Proposed fix
   isResolving: boolean;
+  /** True when the bounded ``useServices`` fetch failed. ``service`` stays undefined. */
+  isError: boolean;
+  /** Error from the bounded ``useServices`` fetch, when it failed. */
+  error: Error | null;
 }
-  const { data: services = EMPTY_SERVICES, isFetched } = useServices({
+  const {
+    data: services = EMPTY_SERVICES,
+    isFetched,
+    isError,
+    error,
+  } = useServices({
     serviceTypes: types,
     enabled,
   });
   return {
     parent,
     service,
     resetKey: cascadeParentResetKey(parent),
     isResolving: enabled && !isFetched,
+    isError: enabled && isError,
+    error: enabled ? (error ?? null) : null,
   };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/framework/src/hooks/useResolvedServiceField.ts` around lines
80 - 96, Expose the fetch error from useServices through the
ResolvedServiceField return value. Destructure the error alongside data and
isFetched, then add it to the returned object so callers can distinguish a
failed service lookup from a successful lookup with no matching service;
preserve the existing service and isResolving behavior.
ui/packages/sep/framework/src/components/HostSelector/StandaloneHostSelector.tsx-73-79 (1)

73-79: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

A failed hosts query disables the selector permanently.

Line 79 disables the Autocomplete when isError is true. Line 73 puts the only retry trigger, refetch(), on onOpen. A disabled Autocomplete never opens, so onOpen never fires. After one hosts-query failure the user cannot recover in this component. The user must remount the page.

Add an explicit retry control, or keep the control enabled on error so onOpen can retry.

🔧 Proposed fix — add a retry action to the error state
+import IconButton from '`@mui/material/IconButton`';
+import InputAdornment from '`@mui/material/InputAdornment`';
+import RefreshIcon from '`@mui/icons-material/Refresh`';
       loading={isLoading}
       loadingText="Loading hosts…"
       noOptionsText="No hosts available"
-      disabled={disabled || isError}
+      disabled={disabled}
       renderInput={(params) => (
         <TextField
           {...params}
           label={label}
           error={isError}
           helperText={
             isError ? (error?.message ?? 'Failed to load hosts') : undefined
           }
+          slotProps={{
+            input: {
+              ...params.InputProps,
+              endAdornment: (
+                <>
+                  {isError && (
+                    <InputAdornment position="end">
+                      <IconButton
+                        size="small"
+                        aria-label="Retry loading hosts"
+                        onClick={() => void refetch()}
+                      >
+                        <RefreshIcon fontSize="small" />
+                      </IconButton>
+                    </InputAdornment>
+                  )}
+                  {params.InputProps.endAdornment}
+                </>
+              ),
+            },
+          }}
         />
       )}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/HostSelector/StandaloneHostSelector.tsx`
around lines 73 - 79, Update the HostSelector Autocomplete error handling so a
hosts-query failure does not permanently block recovery: either remove isError
from disabled or add an explicit retry control that invokes refetch() while the
error state is shown. Preserve disabled behavior for the existing disabled prop
and ensure users can trigger the retry without remounting.
ui/packages/sep/framework/src/components/FreeSoloSelect/freeSoloValue.ts-112-117 (1)

112-117: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Committing the trimmed string blocks trailing spaces during typing.

FreeSoloSelect calls normalizeChange on every keystroke through onInputChange. This function commits trimmed, not the raw input. toDisplayValue then returns that trimmed string as the controlled Autocomplete value, so MUI resets the visible input text to the trimmed form. A user who types a space cannot keep it, because each keystroke removes the trailing space. Custom values that contain spaces become hard to enter.

Use trimmed only for the blank check and for label matching. Commit the raw string.

🐛 Proposed fix
   const trimmed = next.trim();
   if (trimmed === '') {
     return null;
   }
   const match = options.find((o) => getOptionLabel(o) === trimmed);
-  return match ? match.id : trimmed;
+  return match ? match.id : next;
 }

Update the docstring at lines 98-99 to state that a non-empty string is kept verbatim, including surrounding whitespace, and update the test at line 94 to cover a value with an inner space.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/framework/src/components/FreeSoloSelect/freeSoloValue.ts`
around lines 112 - 117, Update normalizeChange around the trimmed blank check so
trimmed is used only to detect empty input and match option labels; return the
raw next string for unmatched non-empty custom values, preserving surrounding
whitespace during typing. Revise the function docstring to state that non-empty
strings are retained verbatim and update the related test to cover a value
containing an inner space.
ui/packages/sep/framework/src/components/HostSelector/HostSelector.tsx-160-160 (1)

160-160: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use a path-aware error lookup for nested field names.

When name is a dotted path such as params.host, errors[name] returns undefined. Use get(errors, name)?.message so the autocomplete field displays its validation error.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/framework/src/components/HostSelector/HostSelector.tsx` at
line 160, Update the fieldError lookup in the HostSelector component to resolve
nested field names through the form error object, using the path-aware get
helper with name before reading message. Preserve the existing
string-or-undefined typing and validation display behavior.
ui/packages/sep/framework/src/utils/extractId.ts-29-41 (1)

29-41: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reject whitespace-only and non-integer strings.

Number(value) coerces a whitespace-only string to 0, so extractId(' ') returns 0 instead of null. Two callers then treat that as a resolvable id:

  • useResolvedServiceField.ts line 78 sets enabled = unresolvedServiceId !== null && ..., so a lookup fires for service id 0 and never matches.
  • SchemaSelector.tsx line 72 computes serviceId from the untrimmed parent, so noService becomes false and useSchemas({ serviceId: 0 }) fires against a nonexistent service.

Number also accepts '1.5' and '0x10', which produce non-integer or unintended ids while ServiceOption.id is an integer.

🐛 Proposed fix
   if (typeof value === 'string' && value !== '') {
-    const n = Number(value);
-    return Number.isFinite(n) ? n : null;
+    const trimmed = value.trim();
+    if (!/^-?\d+$/.test(trimmed)) {
+      return null;
+    }
+    const n = Number(trimmed);
+    return Number.isSafeInteger(n) ? n : null;
   }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/framework/src/utils/extractId.ts` around lines 29 - 41,
Update extractId to reject whitespace-only strings by trimming before
validation, and accept string IDs only when they represent decimal integers.
Preserve finite numeric handling and recursive object id extraction, while
rejecting values such as "1.5", "0x10", and blank strings with null.
ui/packages/sep/framework/src/hooks/useTaskHistoryFiles.ts-41-49 (1)

41-49: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Add a production route for /files/*. PMM’s production nginx configuration defines no /files location or SEP upstream. The Vite proxy applies only in development, so these requests do not reach SEP in production. Add the production proxy or use the deployed SEP route before enabling the download affordance.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/framework/src/hooks/useTaskHistoryFiles.ts` around lines 41 -
49, Update the request path used by the task-history file download flow in the
hook’s queryFn so production requests target the deployed SEP route rather than
relying on the development-only Vite `/files` proxy. Ensure the resulting URL
matches the production routing configuration before exposing the download
affordance.
ui/packages/sep/framework/test/setup.ts-18-18 (1)

18-18: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Two competing Vitest setup files exist in the framework package. The package adds test/setup.ts and tests/setup.ts in sibling directories. Both register the jest-dom matchers, but only the plural one registers afterEach(cleanup). Whichever path vitest.config.ts lists in setupFiles decides whether rendered components are torn down between tests, so a future edit to that config silently changes DOM isolation for the whole package.

  • ui/packages/sep/framework/test/setup.ts#L18-L18: delete this file, because it omits afterEach(cleanup); without cleanup, queries such as getByRole('tab') can match nodes left over from an earlier test.
  • ui/packages/sep/framework/tests/setup.ts#L18-L24: keep this file as the single setup entry point, and confirm setupFiles in ui/packages/sep/framework/vitest.config.ts resolves to this path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/framework/test/setup.ts` at line 18, Remove
ui/packages/sep/framework/test/setup.ts because it lacks afterEach(cleanup).
Retain ui/packages/sep/framework/tests/setup.ts as the sole setup entry point,
and update or verify setupFiles in ui/packages/sep/framework/vitest.config.ts
resolves to that plural-path file.
ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx-245-251 (1)

245-251: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Pass submitError, fieldErrors, and capabilities to the renderEditForm slot.

PluginEditPage maps API failures into submitError and fieldErrors at line 199, then passes both to the default SchemaFormRenderer at lines 257-258. The custom slot invocation omits them. A consumer that supplies renderEditForm therefore cannot render the persistent 422 banner or the inline per-field errors, and the mapSubmitError result becomes dead state on that path.

The sibling page keeps the contract complete. PluginTaskEditPage passes capabilities: schema.capabilities, submitError, and fieldErrors to the same RenderFormSlot (see ui/packages/sep/framework/src/components/SchemaDrivenPlugin/PluginTaskEditPage.tsx). Align this call so both pages honor one slot contract.

🐛 Proposed fix
       {renderEditForm?.({
         sections,
         onSubmit: handleSubmit,
         loading: updateEntity.isPending,
         defaultValues,
+        capabilities: schema.capabilities,
         renderField,
+        submitError,
+        fieldErrors,
       }) ?? (
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx`
around lines 245 - 251, Update the renderEditForm invocation in
SchemaDrivenPlugin to pass capabilities, submitError, and fieldErrors alongside
the existing form props, matching the RenderFormSlot contract and the
PluginTaskEditPage implementation. Reuse the values already derived in
PluginEditPage, including schema.capabilities and the mapped error state.
ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx-300-301 (1)

300-301: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Render related-app tabs and routes for multi-entity schemas. The schema contract allows both entities and related_apps. When both are present, the early return bypasses related-app handling, so related-app paths render no route. Add this handling to the multi-entity branch or enforce mutual exclusion in the schema.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/SchemaDrivenPlugin/SchemaDrivenPlugin.tsx`
around lines 300 - 301, Update the multi-entity handling in SchemaDrivenPlugin
so schemas containing both entities and related_apps still render the
related-app tabs and routes instead of being bypassed by the early return. Reuse
the existing relatedApps and hasRelatedApps flow, or explicitly enforce mutual
exclusion in schema validation if that is the established contract.
ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/ScriptPreviewField.tsx-87-118 (1)

87-118: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Fetch the preview through TanStack Query instead of a manual effect.

This effect performs server-state fetching with a hand-rolled debounce, abort handling, and status machine. The repository guidelines require TanStack Query for all server state and forbid bypassing it for API calls. A useQuery keyed by field.endpoint_url plus the serialized dependency values also gives caching and deduplication across remounts of the same field.

Keep the debounce by deriving a debounced key with useState/useEffect, then pass it into the query key and queryFn.

As per coding guidelines: "Use TanStack Query (useQuery, useMutation) for all server state" and "Do not bypass React Query for API calls; use it for caching, deduplication, and background refetching."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/ScriptPreviewField.tsx`
around lines 87 - 118, Replace the manual fetch effect in ScriptPreviewField
with TanStack Query’s useQuery, using field.endpoint_url and the serialized
dependency values in the query key and queryFn. Preserve the existing debounce
by deriving a debounced dependency key with useState/useEffect, and map query
loading, success, and error data to the existing preview state or rendering
contract while retaining request cancellation through the query signal.

Source: Coding guidelines

ui/packages/sep/framework/src/components/SchemaFormRenderer/utils/validationMapper.ts-59-73 (1)

59-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reject invalid numeric values before coercion.

parseInt('2.5', 10) submits 2. parseFloat('3.14invalid') submits 3.14. The current rules do not reject fractional integer values or non-finite numeric values. SchemaFormRenderer sends this coerced output to the backend.

Add numeric validation with Number.isFinite(). Require Number.isInteger() for integer fields. Use Number() during coercion so invalid input is not silently truncated.

Proposed fix
     case 'integer':
     case 'float': {
+      rules.validate = (value: unknown) => {
+        if (value === '' || value === null || value === undefined) {
+          return true;
+        }
+
+        const numericValue = Number(value);
+        if (!Number.isFinite(numericValue)) {
+          return 'Enter a valid number';
+        }
+        if (field.type === 'integer' && !Number.isInteger(numericValue)) {
+          return 'Enter a whole number';
+        }
+        return true;
+      };
       if (field.ge !== undefined) {
         rules.min = {
-      const num =
-        field.type === 'integer'
-          ? parseInt(String(raw), 10)
-          : parseFloat(String(raw));
-      setAtPath(out, field.name, Number.isNaN(num) ? raw : num);
+      const num = Number(raw);
+      const valid =
+        Number.isFinite(num) &&
+        (field.type !== 'integer' || Number.isInteger(num));
+      setAtPath(out, field.name, valid ? num : raw);

Also applies to: 120-129

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/SchemaFormRenderer/utils/validationMapper.ts`
around lines 59 - 73, Update the numeric validation and coercion logic in the
integer/float handling of validationMapper so values are converted with Number()
rather than parseInt/parseFloat, rejecting non-finite results with
Number.isFinite(). For integer fields, also require Number.isInteger() to reject
fractional input before it reaches the backend; preserve the existing ge/le
range rules.
ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/YamlField.tsx-48-51 (1)

48-51: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use theme typography through sx.

Lines 48-50 hard-code a font family and font size. These values bypass PMM theme settings. Apply the code-font and font-size tokens through sx or a theme-aware styled component.

As per coding guidelines, do not use hard-coded colors, font families, or spacing that bypass the theme; prefer sx with theme tokens, breakpoints, and MUI Typography variants.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/YamlField.tsx`
around lines 48 - 51, Update the input styling in YamlField’s inputProps to use
the theme-aware sx prop with the appropriate typography code-font and font-size
tokens instead of hard-coded fontFamily and fontSize values; preserve the
existing spellCheck setting.

Source: Coding guidelines

ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/FileField.tsx-63-76 (1)

63-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add an accessible name to the file picker button.

IconButton contains only an icon and has no accessible name. Screen-reader users cannot identify the file selection action.

Proposed fix
-                  <IconButton component="label" htmlFor={inputId} edge="end">
+                  <IconButton
+                    aria-label={`Select file for ${field.label}`}
+                    component="label"
+                    htmlFor={inputId}
+                    edge="end"
+                  >
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@ui/packages/sep/framework/src/components/SchemaFormRenderer/fields/FileField.tsx`
around lines 63 - 76, Add an accessible name to the IconButton in the FileField
component, such as an appropriate aria-label describing the file selection
action, while preserving its existing label behavior, icon, and file input
handling.
ui/packages/sep/framework/src/hooks/useTaskLogs.ts-247-260 (1)

247-260: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Log lines without a step are dropped, which conflicts with the execution-events contract.

Line 250 rejects the payload when step is falsy, so a log line with step: '' is discarded. useExecutionEvents treats '' as a valid stepless bucket through STEPLESS_KEY, and ExecutionEventsPanel and LogStepTabs render that bucket as "General". The two streams therefore disagree on the meaning of an empty step. If the backend can emit stepless log lines, this silently loses output. Validate typeof step === 'string' and map '' to the same stepless key that the events path uses.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/framework/src/hooks/useTaskLogs.ts` around lines 247 - 260,
Update the payload validation and key construction in the task-log handler
around offsetsRef to accept empty step strings by validating typeof step ===
'string' instead of treating step as falsy. Normalize step === '' to the shared
STEPLESS_KEY used by useExecutionEvents so stepless logs join the existing
General bucket, while preserving the current offset filtering for other steps.
ui/packages/sep/framework/src/hooks/useExecutionEvents.ts-234-246 (1)

234-246: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Transient stream failures retry without a limit and without user feedback.

onerror returns undefined for every non-sentinel error. fetchEventSource then retries indefinitely. The handler does not set sseError and does not clear sseLoading. If the endpoint fails persistently for a non-401 reason, for example a 500 or a DNS failure, the panel keeps showing the loading state and reconnects forever. Count consecutive failures. After a threshold, set sseError and stop the retry loop by re-throwing.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/packages/sep/framework/src/hooks/useExecutionEvents.ts` around lines 234 -
246, Update the onerror handler in useExecutionEvents to count consecutive
transient stream failures, reset that count after a successful connection, and
after the defined threshold set sseError, clear sseLoading, and re-throw the
error to stop retries. Preserve the existing StreamRetriableAfterRefresh and
StreamFatalError handling, while continuing silent retries below the threshold.

The interim SEP dev proxy could never see SEP_INTERNAL_TOKEN, so every
call to the SEP backend from the migrated pages returned 401.

Two independent faults:

turbo.json declared no passThroughEnv, and Turbo 2.x defaults to
envMode strict, which strips undeclared variables before spawning a
task. vite was therefore started without the variable no matter how it
was exported. Declare the three variables vite.config.ts reads.

Vite exposes .env files to client code as import.meta.env but never to
the config file's own process.env, so the only working setup was an
export in the exact shell launching the dev server, and anything else
fell back to the defaults silently. Load the files explicitly with
loadEnv, keeping real environment variables ahead of file values so CI
and the devcontainer are unaffected.

PMM_SERVER_URL was broken the same way and is fixed by the same change.

Signed-off-by: Ignacio Durand <ignacio.durand@l7informatics.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants